TPT-4456: Project RDMA Linode Interfaces - #736
Conversation
* init * add unit test * lint * nit` * add explict reject * fix firewall id
* init * add unit test * lint * nit` * Add integration tests for RDMA VPC and Subnet * Update fixture and test assertions for RDMA VPC * Create fixture for subnet RDMA * Change rdma fixtures scope --------- Co-authored-by: Ye Chen <yechen@akamai.com>
There was a problem hiding this comment.
🟡 Changes recommended
Preserve VPC API compatibility and correctly gate RDMA tests while selecting an RDMA-capable region.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds GPUDirect RDMA support for VPCs and Linode interfaces across SDK models, APIs, fixtures, and tests.
Changes:
- Adds RDMA VPC types, regional capability, and interface models.
- Supports RDMA interfaces during instance creation.
- Adds unit and integration coverage for RDMA resources.
File summaries
| File | Description |
|---|---|
test/unit/objects/vpc_test.py |
Tests VPC type handling. |
test/unit/objects/linode_test.py |
Tests post-creation RDMA interface rejection. |
test/unit/objects/linode_interface_test.py |
Tests RDMA interface serialization and updates. |
test/unit/groups/linode_test.py |
Tests instance creation with RDMA interfaces. |
test/integration/models/vpc/test_vpc.py |
Tests RDMA VPC and subnet retrieval. |
test/integration/models/linode/interfaces/test_interfaces.py |
Tests RDMA-enabled Linode provisioning. |
test/integration/conftest.py |
Adds RDMA fixtures; test gating and capability-aware region selection need correction. |
test/fixtures/vpcs.json |
Adds VPC type fixture data. |
test/fixtures/vpcs_123456.json |
Adds VPC type fixture data. |
test/fixtures/vpcs_123456_subnets.json |
Adds subnet VPC type fixture data. |
test/fixtures/vpcs_123456_subnets_789.json |
Adds subnet VPC type fixture data. |
test/fixtures/linode_instances_124_interfaces_999.json |
Adds an RDMA interface response fixture. |
linode_api4/objects/vpc.py |
Defines VPC types and response properties. |
linode_api4/objects/region.py |
Adds the GPUDirect RDMA capability. |
linode_api4/objects/linode.py |
Rejects unsupported post-creation RDMA interfaces. |
linode_api4/objects/linode_interfaces.py |
Adds RDMA interface models; public interface-limit documentation needs updating. |
linode_api4/groups/vpc.py |
Adds VPC type creation support but breaks positional argument compatibility. |
Review details
Suppressed comments (1)
linode_api4/objects/linode_interfaces.py:248
- Exposing
rdma_vpcenables the new flow that creates at least eight RDMA interfaces plus a regular interface, butLinodeGroup.instance_createstill documentsinterfacesas limited to three (linode_api4/groups/linode.py:341-343). Update that public docstring to describe the RDMA-specific limit so SDK consumers are not told their supported configuration is invalid.
rdma_vpc: Optional[LinodeInterfaceRDMAVPCOptions] = None
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Some Copilot comments worth a look, and one minor comment about the test case, otherwise looks good to me! |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The integration test region caching in test/integration/conftest.py is currently inconsistent/redundant and can lead to confusing or incorrect region reuse, so it should be made consistent before merging.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
test/integration/conftest.py:480
- TEST_VPC_REGION is computed but the VPC itself is created using a separate get_region() call with a different capability set, so the cached region may not match the actual region used by this session fixture (and may be missing capabilities like Custom VPC IPv4 Ranges). Compute the region once, assign it to TEST_VPC_REGION, and reuse it for the create() call to keep region selection consistent and avoid redundant API calls.
test/integration/models/linode/interfaces/test_interfaces.py:104 - The docstring has a couple of grammatical issues (missing article “an” and inconsistent capitalization of “Linode”). Tightening the wording makes the intent clearer for future readers.
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
16e1eb6
There was a problem hiding this comment.
🟢 Approval recommended
The changes add RDMA VPC/interface support with solid unit and integration coverage, and the remaining feedback is limited to small doc/comment clarity nits.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
linode_api4/groups/vpc.py:70
- The docstring calls
GPUDirect RDMAan "account capability", but this project’s Capability enum represents region capabilities (see Region.Capability). This wording is likely misleading for SDK users; consider referring to it as a region capability (or just "capability") to match the rest of the codebase.
test/integration/models/linode/interfaces/test_interfaces.py:104 - Docstring grammar: "may be needed for RDMA Linode instance" is missing an article, and "linode" should be capitalized for the product name used elsewhere in this file.
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
📝 Description
Merge project rdma interfaces into dev.
✔️ How to Test
To run the integration test, you will need to enable it with
export RUN_RDMA_TESTS=yes